Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOCS-876: Use instantsearch and typesense for tutorials page #1693

Merged
merged 21 commits into from
Sep 5, 2023

Conversation

npentrel
Copy link
Collaborator

@npentrel npentrel commented Aug 22, 2023

  • Add filtering options to tutorials page
  • Add GitHub action to upsert tutorials collection on typesense on push
  • Improve user experience for folks that don't use javascript
  • Order tutorials by featured value (1-8) and then by date (newest first)

https://docs-test.viam.dev/21dbe0b2507868d3e2054a9a9639ae22a7751641/public/tutorials/

@viambot viambot added the safe to build This pull request is marked safe to build from a trusted zone label Aug 22, 2023
@npentrel npentrel force-pushed the algolia branch 4 times, most recently from bea9c54 to 8b929d8 Compare August 24, 2023 22:02
@npentrel npentrel changed the title Algolia tutorials page test Tutorials page with filtering Aug 24, 2023
@npentrel npentrel force-pushed the algolia branch 2 times, most recently from a84b4a7 to e7ddf3c Compare August 28, 2023 16:44
@@ -51,6 +51,8 @@
{{ $manualLink := cond (isset $s.Params "manuallink") $s.Params.manualLink ( cond (isset $s.Params "manuallinkrelref") (relref $s $s.Params.manualLinkRelref) $s.RelPermalink) -}}
{{ $manualLinkTitle := cond (isset $s.Params "manuallinktitle") $s.Params.manualLinkTitle $s.Title -}}
{{ $empty_node := $s.Params.empty_node -}}

{{ warnf "%s %s %s" $s $s.RelPermalink $s.Permalink }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{{ warnf "%s %s %s" $s $s.RelPermalink $s.Permalink }}

layouts/partials/sidebar-tree.html Outdated Show resolved Hide resolved
layouts/partials/sidebar-tree.html Outdated Show resolved Hide resolved
layouts/partials/sidebar-tree.html Outdated Show resolved Hide resolved
@npentrel npentrel force-pushed the algolia branch 2 times, most recently from b53c69c to c322353 Compare August 29, 2023 18:33
@npentrel npentrel requested a review from erh August 29, 2023 20:29
@npentrel npentrel changed the title Tutorials page with filtering DOCS-876: Use instantsearch and typesense for tutorials page Aug 29, 2023
@npentrel npentrel requested a review from mcvella August 29, 2023 21:41
Copy link
Contributor

@mcvella mcvella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -16,8 +16,8 @@ authors: [ "Hazal Mestci" ]
languages: [ "python" ]
viamresources: [ "base", "vision", "camera" ]
level: "Beginner"
date: "18 August 2022"
updated: "11 August 2023"
date: "2022-08-18"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(opt) I don't want to block bc I don't think it matters too much but I will say american ppl might get confused by this date format

Suggested change
date: "2022-08-18"
date: "08-18-2022"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not just american people, but in this case this is how Go expects the date to be formatted, so it is what it is

Copy link
Collaborator

@sguequierre sguequierre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM no issues -- a couple comments with suggestions in case you want to implement

cost: "0"
no_list: true
featured: 4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how much we've featured it on socials?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an optional value that determines the sorting. So this one should appear in 4th position. I could change this to weight to be more in line with how we think about this in the rest of the docs?

@@ -15,13 +15,16 @@ tags: ["tutorial"]
draft: true # Change this when you're ready
authors: [] # Your Name
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would add "featured" to template here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

cacheSearchResultsForSeconds: 2 * 60, // Cache search results from server. Defaults to 2 minutes. Set to 0 to disable caching.
},
// The following parameters are directly passed to Typesense's search API endpoint.
// So you can pass any parameters supported by the search endpoint below.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems useful but I would suggest having more info here if this tip will be important for understanding search functionality in the future - is this endpoint what people are using/querying in the search bar of docs? and code here is for that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the search bar works similar but with a different api key and different parameters. It's two different collections that the data for searching is stored in. This code dynamically adds the search sidebar and the tutorial cards to the page by accessing the collection and searching for tutorials that fit the search criteria and then adding them to the page

width: 100%;
}

.ais-Pagination-item {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(opt) If you know how to toggle button size on square buttons, it seems like the buttons might be smaller than they appear.

I don't know if this is the right object (here where I'm commenting) but I did notice at least in staging a lot of the buttons in the tutorials index/nav bar don't work for me if I try to click them, at first, they only have a small surface area. Tiny issue and non blocking, but if you could make the buttons slightly bigger it would be completely unnoticeable.

Only the buttons that look like << and >> are easily clickable, the rest I have to hover for a bit. Not sure how to fix, but might be a browser related issue on my end?

This bar is what I'm talking about:
Screenshot 2023-08-31 at 10 32 44 AM

Copy link
Contributor

@andf-viam andf-viam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some high-level comments, nothing blocking, LGTM.

  • Replacing left-nav is a cool conservation of real estate, but may confuse users who are exploring the site, and now cannot return to the rest of it. If I want to look at tutorials to get a feel of available options, but then want to look at services to do same, I cannot unless I explicitly select a (any) tutorial to restore the left-nav (or know to click icon in upper-left). This will likely confuse.
  • Having checkboxes disappear on the left, instead of grey-out, looks like a browser problem or that something is broken. You mean: checkboxes removed do not have a valid union with current selection(s). They see: I clicked one thing and the other got lost, and I don't remember what it said to know if I wanted it or not. Greyed-out makes this clear: not compatible with current selection(s).
  • Approx cost should include the currency. Just a $ before the first - or both - text inputs should be fine!

@npentrel
Copy link
Collaborator Author

  • Page navigation is fixed - good points Sierra
  • for the left nav - would you prefer to keep the nav there and have the filters at the top of the page? I think that makes the page cluttered. Could add a link in the left nav to return to the docs page. Any other ideas?
  • I like the grayed out approach but will do that work in a follow up ticket
  • Added the $ sign to the approximate cost header (to change the rest requires creating a custom component, which I may do with this ticket but don't want to look into right now)

@andf-viam
Copy link
Contributor

@npentrel Top would work fine, and I think make sense! You are already capping presented tuts at 12 per page, so I don't think an options bar at top would be too much for content pane -- but restoring user-expected left-nav functionality outweighs in my opinion. Thanks for considering!

@JessamyT
Copy link
Collaborator

  • I agree that left nav should still be available, so I like the idea of filters up top instead.
  • I worry that, since almost all the tutorials are currently categorized as beginner level, folks won't actually find super simple starter tutorials as easily as they would with the old format.
    • Maybe we could have a super obvious "getting started" area somewhere, either here or elsewhere, to make up for it?
  • The cost bars spill into the page on my default screen width:
    image
    (which I suppose would be rectified if filters moved to the top)

@npentrel npentrel force-pushed the algolia branch 5 times, most recently from 18d3557 to e1ae3c3 Compare September 5, 2023 15:41
@viambot
Copy link
Member

viambot commented Sep 5, 2023

You can view a rendered version of the docs from this PR at https://docs-test.viam.dev/3a2daa4191862927e16084282e0e85e89163dd8d/public

@npentrel npentrel merged commit 29585ab into viamrobotics:main Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to build This pull request is marked safe to build from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants